-
-
Notifications
You must be signed in to change notification settings - Fork 702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added concurrency limit for markdown file processing #3481
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request introduces modifications to the AsyncAPI website project, focusing on dependency management and concurrency control for markdown file processing. A new function, Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3481--asyncapi-website.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/markdown/check-markdown.test.js (3)
83-86
: Avoid usingawait
inside a loop for better performanceUsing
await
inside a loop can cause the iterations to run sequentially, which may impact performance. Consider usingPromise.all
to execute these asynchronous operations concurrently.Apply this diff to refactor the code:
- for (let i = 0; i < 20; i++) { - await fs.writeFile( - path.join(tempDir, `test${i}.md`), - '---\ntitle: Test\n---' - ); - } + const writePromises = []; + for (let i = 0; i < 20; i++) { + writePromises.push( + fs.writeFile( + path.join(tempDir, `test${i}.md`), + '---\ntitle: Test\n---' + ) + ); + } + await Promise.all(writePromises);🧰 Tools
🪛 eslint
[error] 83-86: Unexpected
await
inside a loop.(no-await-in-loop)
[error] 83-86: Replace
⏎··········path.join(tempDir,·
test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········
withpath.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
117-121
: Format theauthors
array to comply with Prettier guidelinesThe
authors
array formatting does not align with Prettier's style rules, which can affect code readability.Apply this diff to reformat the array:
- authors: [ - { name: 'John' }, - { photo: 'jane.jpg' }, - { name: 'Bob', photo: 'bob.jpg', link: 'not-a-url' }, - ], + authors: [{ name: 'John' }, { photo: 'jane.jpg' }, { name: 'Bob', photo: 'bob.jpg', link: 'not-a-url' }],🧰 Tools
🪛 eslint
[error] 117-121: Replace
⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······],
with{·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
151-153
: Simplify theexpect
statement formattingThe
expect
statement can be reformatted to improve readability and comply with style guidelines.Apply this diff:
- expect(mockConsoleLog).toHaveBeenCalledWith( - expect.stringContaining('Errors in file invalid.md:'), - ); + expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:'));🧰 Tools
🪛 eslint
[error] 151-153: Replace
⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎····
withexpect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
scripts/markdown/check-markdown.js (4)
145-150
: Improve function parameter formatting for readabilityThe parameters of the
checkMarkdownFiles
function can be formatted on a single line to enhance readability.Apply this diff:
- async function checkMarkdownFiles( - folderPath, - validateFunction, - limit, - relativePath = '' - ) { + async function checkMarkdownFiles(folderPath, validateFunction, limit, relativePath = '') {🧰 Tools
🪛 eslint
[error] 145-150: Replace
⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎
withfolderPath,·validateFunction,·limit,·relativePath·=·''
(prettier/prettier)
166-171
: Simplify recursive function call formattingThe recursive call to
checkMarkdownFiles
can be formatted on a single line for better readability.Apply this diff:
- await checkMarkdownFiles( - filePath, - validateFunction, - limit, - relativeFilePath - ); + await checkMarkdownFiles(filePath, validateFunction, limit, relativeFilePath);🧰 Tools
🪛 eslint
[error] 166-171: Replace
⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········
withfilePath,·validateFunction,·limit,·relativeFilePath
(prettier/prettier)
173-173
: Add a space aftertry
keywordAdding a space after the
try
keyword enhances code readability.Apply this diff:
- try{ + try {🧰 Tools
🪛 eslint
[error] 173-173: Insert
·
(prettier/prettier)
185-185
: Insert a space after}catch
For consistency and readability, add a space between
}
andcatch
.Apply this diff:
- }catch (error) { + } catch (error) {🧰 Tools
🪛 eslint
[error] 185-185: Insert
·
(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json
(2 hunks)scripts/markdown/check-markdown.js
(3 hunks)tests/markdown/check-markdown.test.js
(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 78-78: Replace resolve
with (resolve)
(prettier/prettier)
[error] 78-78: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 82-82: Unary operator '++' used.
(no-plusplus)
[error] 83-86: Unexpected await
inside a loop.
(no-await-in-loop)
[error] 83-86: Replace ⏎··········path.join(tempDir,·
test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········
with path.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 100-100: Replace group
with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10
with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
[error] 117-121: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······],
with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 129-129: Delete ,
(prettier/prettier)
[error] 138-138: Delete ··
(prettier/prettier)
[error] 143-146: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······
---\ntitle:·Invalid·Blog\n---,⏎····
with path.join(tempDir,·'invalid.md'),·
---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 151-153: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎····
with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 164-164: Delete ,
(prettier/prettier)
[error] 172-172: Delete ,
(prettier/prettier)
[error] 179-181: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 189-192: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······
---\ntitle:·Skipped·File\n---,⏎····
with path.join(referenceSpecDir,·'skipped.md'),·
---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 199-201: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······),
with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 212-214: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 215-218: Replace ⏎······expect.stringContaining(
Error·in·directory),⏎······expect.any(Error),⏎····
with expect.stringContaining(
Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 230-233: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎····
with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 250-250: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 23-23: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.
with ⏎······
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎··
with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎··
with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 145-150: Replace ⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎
with folderPath,·validateFunction,·limit,·relativePath·=·''
(prettier/prettier)
[error] 166-171: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········
with filePath,·validateFunction,·limit,·relativeFilePath
(prettier/prettier)
[error] 173-173: Insert ·
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 185-185: Insert ·
(prettier/prettier)
🔇 Additional comments (2)
package.json (2)
93-93
: Dependency p-limit
correctly added
The addition of "p-limit": "^6.1.0"
to dependencies
is appropriate for managing concurrency in markdown processing.
155-155
: Confirm the necessity of fast-xml-parser
in devDependencies
The fast-xml-parser
package has been added to devDependencies
. Ensure that this package is required for development purposes and is not unnecessarily included.
Run the following script to check for its usage:
✅ Verification successful
Let me check the usage context of fast-xml-parser
to verify if it's appropriately placed in devDependencies.
Let me check the build-rss script to understand the complete picture.
fast-xml-parser
is correctly placed in devDependencies
The package is only used in test files (tests/build-rss.test.js
) to parse and validate the XML output during testing. The actual RSS feed generation uses jgexml/json2xml
package, making fast-xml-parser
purely a development dependency for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of 'fast-xml-parser' in the codebase.
# Test: Find all import statements using 'fast-xml-parser'.
# Expect: Only files that require it for development tasks.
rg "require\('fast-xml-parser'\)|import.+from 'fast-xml-parser'"
Length of output: 152
Script:
#!/bin/bash
# Check the content of the test file to understand how fast-xml-parser is used
cat tests/build-rss.test.js
Length of output: 5911
Script:
#!/bin/bash
# Check the content of the RSS build script
cat scripts/build-rss.js
Length of output: 3431
groups.forEach(group => { | ||
const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length; | ||
expect(concurrentExecutions).toBeLessThanOrEqual(5); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the calculation of concurrent executions in the test
The current method of calculating concurrentExecutions
is inaccurate because mockValidateFunction.mock.calls
contains the arguments passed to the mock function, not the timestamps. As a result, call[0]
is undefined.
Consider tracking active executions to verify concurrency limits. Here's an alternative approach:
- groups.forEach(group => {
- const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length;
- expect(concurrentExecutions).toBeLessThanOrEqual(5);
- });
+ let activeCount = 0;
+ let maxActiveCount = 0;
+ const mockValidateFunction = jest.fn().mockImplementation(async () => {
+ activeCount++;
+ if (activeCount > maxActiveCount) maxActiveCount = activeCount;
+ await new Promise(resolve => setTimeout(resolve, 50));
+ activeCount--;
+ });
+ // After execution
+ expect(maxActiveCount).toBeLessThanOrEqual(5);
This approach tracks the maximum number of concurrent executions during the test.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 100-100: Replace group
with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10
with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
scripts/markdown/check-markdown.js
Outdated
return false; | ||
} | ||
try { | ||
URL(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect usage of URL
constructor
The URL
constructor should be called with the new
keyword to create a new URL object.
Apply this diff to correct the function:
- URL(str);
+ new URL(str);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
URL(str); | |
new URL(str); |
1e8c182
to
42fe248
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
scripts/markdown/check-markdown.js (3)
10-36
: Consider extracting magic numbers as named constants.The implementation is robust, but readability could be improved by extracting magic numbers into named constants.
+const DEFAULT_CONCURRENCY_LIMIT = 10; + function getConcurrencyLimit() { const envLimit = process.env.MARKDOWN_CONCURRENCY_LIMIT; // If no env var is set, return default if (envLimit === undefined) { - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } // Attempt to parse the environment variable const parsedLimit = parseInt(envLimit, 10); // Validate the parsed limit if (Number.isNaN(parsedLimit)) { console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } // Check for non-positive integers if (parsedLimit <= 0) { console.warn( `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` ); - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } return parsedLimit; }🧰 Tools
🪛 eslint
[error] 23-23: Replace
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.
with⏎······
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
145-184
: Enhance error handling and documentation for concurrent processing.While the implementation is good, consider adding:
- Documentation about the concurrency behavior
- Error handling specific to the concurrency limiter
/** * Recursively checks markdown files in a folder and validates their frontmatter. * @param {string} folderPath - The path to the folder to check. * @param {Function} validateFunction - The function used to validate the frontmatter. * @param {string} [relativePath=''] - The relative path of the folder for logging purposes. * @param {import('p-limit').default} limit - Concurrency limiter. + * @throws {Error} When the concurrency limiter fails or when file operations fail + * @example + * // Process files with a concurrency limit of 5 + * const limit = pLimit(5); + * await checkMarkdownFiles(folderPath, validateFn, limit); */🧰 Tools
🪛 eslint
[error] 167-167: Delete
··
(prettier/prettier)
176-178
: Enhance error messages with more context.Consider improving error messages by including more context and using structured logging.
- console.error(`Error processing file ${relativeFilePath}:`, error); + console.error(`Error processing markdown file ${relativeFilePath} (validation failed):`, { + error: error.message, + code: error.code, + stack: error.stack + }); throw error; - console.error(`Error in directory ${folderPath}:`, err); + console.error(`Failed to process markdown files in directory ${folderPath}:`, { + error: err.message, + code: err.code, + stack: err.stack + }); throw err; - console.error('Failed to validate markdown files:', error); + console.error('Markdown validation process failed:', { + error: error.message, + code: error.code, + stack: error.stack + });Also applies to: 185-187, 207-209
tests/markdown/check-markdown.test.js (2)
72-109
: Improve concurrency test reliability and readability.The concurrency test could be more reliable and readable with these improvements:
- Use a more descriptive test name
- Add assertions for minimum concurrent executions
- Use constants for test values
- it('respects concurrency limit during file processing', async () => { + it('should process files concurrently while respecting the configured limit', async () => { + const CONCURRENCY_LIMIT = 5; + const TOTAL_FILES = 20; + const PROCESSING_TIME_MS = 50; let activeCount = 0; let maxActiveCount = 0; const mockValidateFunction = jest.fn().mockImplementation(async () => { activeCount++; try { - // Track the maximum number of concurrent executions if (activeCount > maxActiveCount) { maxActiveCount = activeCount; } - // Simulate some processing time - await new Promise(resolve => setTimeout(resolve, 50)); + await new Promise(resolve => setTimeout(resolve, PROCESSING_TIME_MS)); } finally { activeCount--; } }); const writePromises = []; - for (let i = 0; i < 20; i++) { + for (let i = 0; i < TOTAL_FILES; i++) { writePromises.push( fs.writeFile( path.join(tempDir, `test${i}.md`), '---\ntitle: Test\n---' ) ); } await Promise.all(writePromises); - const limit = pLimit(5); // Set limit to 5 + const limit = pLimit(CONCURRENCY_LIMIT); await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); - // Verify that the maximum number of concurrent executions never exceeds the limit - expect(maxActiveCount).toBeLessThanOrEqual(5); + // Verify concurrent execution bounds + expect(maxActiveCount).toBe(CONCURRENCY_LIMIT); - // Verify that the mock validate function was called for all files - expect(mockValidateFunction).toHaveBeenCalledTimes(20); + expect(mockValidateFunction).toHaveBeenCalledTimes(TOTAL_FILES); });🧰 Tools
🪛 eslint
[error] 76-76: Unary operator '++' used.
(no-plusplus)
[error] 82-82: Delete
··········
(prettier/prettier)
[error] 84-84: Replace
resolve
with(resolve)
(prettier/prettier)
[error] 84-84: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 86-86: Unary operator '--' used.
(no-plusplus)
[error] 89-89: Delete
····
(prettier/prettier)
[error] 91-91: Unary operator '++' used.
(no-plusplus)
[error] 92-97: Replace
⏎············fs.writeFile(⏎··············path.join(tempDir,·
test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········
withfs.writeFile(path.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 100-100: Delete
····
(prettier/prettier)
[error] 103-103: Delete
····
(prettier/prettier)
[error] 106-106: Delete
····
(prettier/prettier)
153-170
: Improve test organization and naming.Consider organizing related test cases using nested describe blocks and more descriptive test names.
- it('returns multiple validation errors for invalid blog frontmatter', async () => { + describe('Blog Frontmatter Validation', () => { + it('should return all validation errors when multiple fields are invalid', async () => { const frontmatter = { title: 123, date: 'invalid-date', type: 'blog', tags: 'not-an-array', cover: ['not-a-string'], authors: { name: 'John Doe' }, }; const errors = validateBlogs(frontmatter); expect(errors).toEqual([ 'Invalid date format: invalid-date', 'Tags should be an array', 'Cover must be a string', 'Authors should be an array' ]); - }); + }); + });🧰 Tools
🪛 eslint
[error] 160-160: Delete
,
(prettier/prettier)
[error] 168-168: Delete
,
(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js
(3 hunks)tests/markdown/check-markdown.test.js
(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 76-76: Unary operator '++' used.
(no-plusplus)
[error] 82-82: Delete ··········
(prettier/prettier)
[error] 84-84: Replace resolve
with (resolve)
(prettier/prettier)
[error] 84-84: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 86-86: Unary operator '--' used.
(no-plusplus)
[error] 89-89: Delete ····
(prettier/prettier)
[error] 91-91: Unary operator '++' used.
(no-plusplus)
[error] 92-97: Replace ⏎············fs.writeFile(⏎··············path.join(tempDir,·
test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········
with fs.writeFile(path.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 100-100: Delete ····
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 127-127: Delete ,
(prettier/prettier)
[error] 136-136: Delete ··
(prettier/prettier)
[error] 141-144: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······
---\ntitle:·Invalid·Blog\n---,⏎····
with path.join(tempDir,·'invalid.md'),·
---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 160-160: Delete ,
(prettier/prettier)
[error] 168-168: Delete ,
(prettier/prettier)
[error] 175-177: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 185-188: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······
---\ntitle:·Skipped·File\n---,⏎····
with path.join(referenceSpecDir,·'skipped.md'),·
---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 195-197: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······),
with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 208-210: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 211-214: Replace ⏎······expect.stringContaining(
Error·in·directory),⏎······expect.any(Error),⏎····
with expect.stringContaining(
Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 226-229: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎····
with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 246-246: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 23-23: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.
with ⏎······
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 45-45: Do not use 'new' for side effects.
(no-new)
[error] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎··
with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎··
with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 167-167: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
scripts/markdown/check-markdown.js (1)
44-49
:
Fix incorrect usage of URL constructor.
The URL
constructor must be called with the new
keyword.
function isValidURL(str) {
try {
- URL(str);
+ new URL(str);
return true;
} catch (err) {
return false;
}
}
Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 45-45: Do not use 'new' for side effects.
(no-new)
e5463b5
to
c8e7e44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
scripts/markdown/check-markdown.js (3)
8-38
: Consider enhancing error handling and readability.The function logic is sound, but could be improved:
+// Constants for configuration +const DEFAULT_CONCURRENCY_LIMIT = 10; +const MIN_CONCURRENCY_LIMIT = 1; + function getConcurrencyLimit() { const envLimit = process.env.MARKDOWN_CONCURRENCY_LIMIT; if (envLimit === undefined) { return DEFAULT_CONCURRENCY_LIMIT; } const parsedLimit = parseInt(envLimit, 10); if (Number.isNaN(parsedLimit)) { - console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); + console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Expected a number, got ${typeof envLimit}. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.`); return DEFAULT_CONCURRENCY_LIMIT; } - if (parsedLimit <= 0) { + if (parsedLimit < MIN_CONCURRENCY_LIMIT) { console.warn( - `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` + `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer (>= ${MIN_CONCURRENCY_LIMIT}). Received: ${parsedLimit}. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.` ); return DEFAULT_CONCURRENCY_LIMIT; } return parsedLimit; }🧰 Tools
🪛 eslint
[error] 25-25: Replace
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.
with⏎······
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
145-195
: Consider refactoring for better maintainability.The function handles multiple responsibilities and could benefit from separation of concerns.
Consider extracting the file processing logic into a separate function:
+async function processMarkdownFile(filePath, relativeFilePath, validateFunction) { + const fileContent = await fs.readFile(filePath, 'utf-8'); + const { data: frontmatter } = matter(fileContent); + + const errors = validateFunction(frontmatter); + if (errors) { + console.log(`Errors in file ${relativeFilePath}:`); + errors.forEach((error) => console.log(` - ${error}`)); + process.exitCode = 1; + } +} + async function checkMarkdownFiles(folderPath, validateFunction, limit, relativePath = '') { try { const files = await fs.readdir(folderPath); const filePromises = files.map(async (file) => { // ... existing path handling code ... if (stats.isDirectory()) { await checkMarkdownFiles(filePath, validateFunction, limit, relativeFilePath); } else if (path.extname(file) === '.md') { try { - await limit(async () => { - const fileContent = await fs.readFile(filePath, 'utf-8'); - const { data: frontmatter } = matter(fileContent); - - const errors = validateFunction(frontmatter); - if (errors) { - console.log(`Errors in file ${relativeFilePath}:`); - errors.forEach((error) => console.log(` - ${error}`)); - process.exitCode = 1; - } - }); + await limit(() => processMarkdownFile(filePath, relativeFilePath, validateFunction)); } catch (error) { console.error(`Error processing markdown file ${relativeFilePath}:`, { error: error.message, code: error.code, stack: error.stack }); throw error; } } }); await Promise.all(filePromises); } catch (err) { console.error(`Failed to process markdown files in directory ${folderPath}:`, { error: err.message, code: err.code, stack: err.stack }); throw err; } }🧰 Tools
🪛 eslint
[error] 167-167: Delete
··
(prettier/prettier)
201-221
: Enhance error handling in main function.The error handling could be more specific and informative.
async function main() { try { const concurrencyLimit = getConcurrencyLimit(); console.log(`Configured markdown processing with concurrency limit: ${concurrencyLimit}`); const limit = pLimit(concurrencyLimit); await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs, limit, ''), checkMarkdownFiles(blogsFolderPath, validateBlogs, limit, '') ]); } catch (error) { console.error('Markdown validation process failed:', { + message: 'Failed to validate markdown files. Check the error details below.', error: error.message, code: error.code, + path: error.path, stack: error.stack }); process.exit(1); } }tests/markdown/check-markdown.test.js (2)
72-111
: Improve concurrency test reliability and readability.The concurrency test could be more reliable and easier to understand.
it('should process files concurrently while respecting the configured limit', async () => { + // Test configuration const CONCURRENCY_LIMIT = 5; const TOTAL_FILES = 20; const PROCESSING_TIME_MS = 50; + const TEST_CONTENT = '---\ntitle: Test\n---'; + + // Concurrency tracking let activeCount = 0; let maxActiveCount = 0; + let processedCount = 0; + const mockValidateFunction = jest.fn().mockImplementation(async () => { activeCount++; try { - // Track the maximum number of concurrent executions if (activeCount > maxActiveCount) { maxActiveCount = activeCount; } - // Simulate some processing time await new Promise(resolve => setTimeout(resolve, PROCESSING_TIME_MS)); + processedCount++; } finally { activeCount--; } }); + // Create test files const writePromises = []; for (let i = 0; i < TOTAL_FILES; i++) { writePromises.push( - fs.writeFile( - path.join(tempDir, `test${i}.md`), - '---\ntitle: Test\n---' - ) + fs.writeFile(path.join(tempDir, `test${i}.md`), TEST_CONTENT) ); } await Promise.all(writePromises); const limit = pLimit(CONCURRENCY_LIMIT); await checkMarkdownFiles(tempDir, mockValidateFunction, limit, ''); - // Verify concurrent execution bounds expect(maxActiveCount).toBe(CONCURRENCY_LIMIT); - expect(mockValidateFunction).toHaveBeenCalledTimes(TOTAL_FILES); + expect(processedCount).toBe(TOTAL_FILES); });🧰 Tools
🪛 eslint
[error] 79-79: Unary operator '++' used.
(no-plusplus)
[error] 85-85: Delete
··········
(prettier/prettier)
[error] 87-87: Replace
resolve
with(resolve)
(prettier/prettier)
[error] 87-87: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 89-89: Unary operator '--' used.
(no-plusplus)
[error] 92-92: Delete
····
(prettier/prettier)
[error] 94-94: Unary operator '++' used.
(no-plusplus)
[error] 95-100: Replace
⏎············fs.writeFile(⏎··············path.join(tempDir,·
test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········
withfs.writeFile(path.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 103-103: Delete
····
(prettier/prettier)
[error] 106-106: Delete
····
(prettier/prettier)
[error] 109-109: Delete
····
(prettier/prettier)
176-184
: Enhance error test case assertions.The error test case could have more specific assertions.
it('logs error to console when an error occurs in checkMarkdownFiles', async () => { const invalidFolderPath = path.join(tempDir, 'non-existent-folder'); + const expectedError = new Error('ENOENT'); await expect( checkMarkdownFiles(invalidFolderPath, validateBlogs, '', pLimit(10)), - ).rejects.toThrow('ENOENT'); + ).rejects.toThrow(expectedError); - expect(mockConsoleError.mock.calls[0][0]).toContain('Error in directory'); + expect(mockConsoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to process markdown files in directory'), + expect.objectContaining({ + error: expectedError.message, + code: 'ENOENT' + }) + ); });🧰 Tools
🪛 eslint
[error] 179-181: Replace
⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····
withcheckMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js
(3 hunks)tests/markdown/check-markdown.test.js
(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 79-79: Unary operator '++' used.
(no-plusplus)
[error] 85-85: Delete ··········
(prettier/prettier)
[error] 87-87: Replace resolve
with (resolve)
(prettier/prettier)
[error] 87-87: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 89-89: Unary operator '--' used.
(no-plusplus)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Unary operator '++' used.
(no-plusplus)
[error] 95-100: Replace ⏎············fs.writeFile(⏎··············path.join(tempDir,·
test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········
with fs.writeFile(path.join(tempDir,·
test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 109-109: Delete ····
(prettier/prettier)
[error] 129-129: Delete ,
(prettier/prettier)
[error] 138-138: Delete ··
(prettier/prettier)
[error] 143-146: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······
---\ntitle:·Invalid·Blog\n---,⏎····
with path.join(tempDir,·'invalid.md'),·
---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 163-163: Delete ,
(prettier/prettier)
[error] 171-171: Delete ,
(prettier/prettier)
[error] 173-173: Insert ;
(prettier/prettier)
[error] 179-181: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 189-192: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······
---\ntitle:·Skipped·File\n---,⏎····
with path.join(referenceSpecDir,·'skipped.md'),·
---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 198-198: Replace expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
with ⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')⏎····
(prettier/prettier)
[error] 208-210: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎····
with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 211-214: Replace ⏎······expect.stringContaining(
Error·in·directory),⏎······expect.any(Error),⏎····
with expect.stringContaining(
Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 226-226: Insert ·
(prettier/prettier)
[error] 243-243: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 25-25: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.
with ⏎······
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 47-47: Do not use 'new' for side effects.
(no-new)
[error] 123-126: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎··
with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 167-167: Delete ··
(prettier/prettier)
try { | ||
new URL(str); | ||
return true; | ||
} catch (err) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix URL constructor usage.
The current implementation triggers an eslint error for using 'new' for side effects.
function isValidURL(str) {
try {
- new URL(str);
+ return Boolean(new URL(str));
- return true;
} catch (err) {
return false;
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
try { | |
new URL(str); | |
return true; | |
} catch (err) { | |
return false; | |
} | |
try { | |
return Boolean(new URL(str)); | |
} catch (err) { | |
return false; | |
} |
🧰 Tools
🪛 eslint
[error] 47-47: Do not use 'new' for side effects.
(no-new)
@Aditya0732 Pretty good stuff from what I can see. It seems from tests that it fails at some module import error. In that case, we would have to wait for TypeScript migration of the scripts in #3423 |
Right. Is someone working on it? |
Yeah, someone's already working on it. |
Implemented Configurable Concurrency Limit for Markdown File Processing
📝 Description
This pull request introduces a configurable concurrency limit for markdown file processing to improve performance, prevent resource exhaustion, and provide more controlled file validation.
🎯 Problem Statement
In large documentation projects, processing markdown files can:
✨ Features
MARKDOWN_CONCURRENCY_LIMIT
environment variablep-limit
for controlled concurrent file processing🚀 Implementation Details
Concurrency Limit Configuration
MARKDOWN_CONCURRENCY_LIMIT
Example Configuration
# Set custom concurrency limit MARKDOWN_CONCURRENCY_LIMIT=20 npm run validate-markdown
💻 Code Changes
New Function:
getConcurrencyLimit()
🌟 Benefits
🧪 Testing
Added comprehensive test cases for:
Verified behavior across various input scenarios
📋 Configuration Guidance
🔍 Potential Future Improvements
🚧 Breaking Changes
📦 Dependencies
p-limit
package for concurrency management🤝 Code Review Checklist
ℹ️ Additional Notes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests